Skip to content

feat(ffi): initial C# bindings (connector only)#423

Merged
Benoît Cortier (CBenoit) merged 47 commits intomasterfrom
create-ffi-bindings
Apr 5, 2024
Merged

feat(ffi): initial C# bindings (connector only)#423
Benoît Cortier (CBenoit) merged 47 commits intomasterfrom
create-ffi-bindings

Conversation

@irvingoujAtDevolution
Copy link
Copy Markdown
Contributor

@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) commented Mar 25, 2024

Created skeleton for FFI.

Connector module is working now. Please see ffi\dotnet\Devolutions.IronRdp.ConnectExample
image

Note: this is a draft, we'll need to discuss more about

  1. how would we like to expose rustls.
  2. how would we like to handle generics like framed and static channels.
  3. should we expose async interface? or just stick with the sync one?

There are some imperfections/todo/fixme for now, my apologies.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 25, 2024

Coverage Report 🤖 ⚙️

Past:
Total lines: 29575
Covered lines: 17244 (58.31%)

New:
Total lines: 29575
Covered lines: 17244 (58.31%)

Diff: +0.00%

[this comment will be updated automatically]

Comment thread ffi/dotnet/Devolutions.IronRdp.ConnectExample/Program.cs Outdated
Comment thread ffi/justfile Outdated
Comment thread ffi/src/connector/mod.rs Outdated
Comment thread ffi/src/pdu.rs
@irvingoujAtDevolution
Copy link
Copy Markdown
Contributor Author

irvingouj@Devolutions (irvingoujAtDevolution) commented Mar 26, 2024

formatter seems not working anymore, it does not tell you where the format is wrong, neither could cargo +stable fmt --all fix it.

update: somehow formatter require me to fix a place I never edited, it is inside crates/ironrdp-testsuite-core/tests/pcb.rs

@CBenoit
Copy link
Copy Markdown
Member

Benoît Cortier (CBenoit) commented Mar 27, 2024

  • how would we like to expose rustls.

We don’t expose rustls, we’ll use SslStream in C#.
The API we expose should be sans-IO and we only push and pull bytes from a state machine. This means that as a rule of thumb, we don’t perform any I/O in the ffi crate (not even using TcpStream).

https://sans-io.readthedocs.io/how-to-sans-io.html#why-write-i-o-free-protocol-implementations

IronRDP is already largely following this philosophy (and IronVNC too). We do expose higher level helpers to simplify things in upper layers, but this is not part of the "core crates".

  • how would we like to handle generics like framed and static channels.

Let’s not expose the API for static channels like we do in Rust. I think we can safely hardcode the supported channels like this. We’re not going to implement static channels in C# either, so there is no need to care too deeply about genericity here.

As for "Framed", it’s not supposed to be exposed. This is a higher level helper for Rust code.

  • should we expose async interface? or just stick with the sync one?

We stick to the "core" API, which is "sans-IO" and thus do not require choosing between async or not.

However, we should provide an higher level API which may be async.
The additional handwritten classes or addons should live in a src/ folder like in Picky: https://github.com/Devolutions/picky-rs/tree/master/ffi/dotnet/Devolutions.Picky/src

However, let’s stick to the standard C# library in the higher level helpers. I don’t think we need much more than that and it will save us maintenance time. If a fancy dependency is required, let’s handle that downstream (in RDM).

Comment thread Cargo.lock Outdated
Comment on lines +143 to +147
[[package]]
name = "anyhow"
version = "1.0.80"
version = "1.0.81"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5ad32ce52e4161730f7098c077cd2ed6229b5804ccf99e5366be1ab72a98b4e1"
checksum = "0952808a6c2afd1aa8947271f3a60f1a6763c7b912d210184c5149b5cf147247"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Avoid to update the Cargo.lock more than necessary. We use dependabot to update it piece by piece in a controlled way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated!

Comment thread xtask/src/cli.rs Outdated
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a .gitattributes file like we have in Picky?
https://github.com/Devolutions/picky-rs/blob/master/.gitattributes

Comment thread ffi/dotnet/Devolutions.IronRdp.ConnectExample/Program.cs
Comment thread ffi/dotnet/Devolutions.IronRdp/src/Framed.cs Outdated
Comment thread ffi/dotnet/Devolutions.IronRdp/src/Framed.cs Outdated
Comment thread ffi/src/connector/config.rs Outdated
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Comment thread ffi/src/lib.rs
Comment thread ffi/src/connector/mod.rs Outdated
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Co-authored-by: Benoît Cortier <bcortier@proton.me>
Comment thread ffi/dotnet/Devolutions.IronRdp/src/Framed.cs
Comment thread ffi/dotnet/Devolutions.IronRdp/src/Framed.cs Outdated
Comment thread ffi/dotnet/Devolutions.IronRdp/src/Framed.cs Outdated
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants